✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.44%. Comparing base (4a0fe55) to head (767ca64).
⚠️ Report is 19 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1109      +/-   ##
==========================================
+ Coverage   83.22%   83.44%   +0.22%     
==========================================
  Files          66       66              
  Lines        2795     2790       -5     
  Branches      332      332              
==========================================
+ Hits         2326     2328       +2     
+ Misses        423      413      -10     
- Partials       46       49       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I could do with some clarification of why the repository name didn't exactly match. Until #1043 is merged, git-proxy can't support multiple forks of the same repo (project or organisation name is different, repo name the same). This PR addresses one or two of those but, by including only a subset of the changes from #1043 it doesn't give you the ability work with multiple forks through the API or UI.

The discrepancy here is due to testing against GitLab, which allows arbitrary display names for repos. For example, I have a repo named Git Proxy Infra, but the actual repo URL in the git push is git-proxy-infra. So the display name and the slug used in the push don't exactly match — which causes issues with how git-proxy currently resolves repositories. People tend to enter the name they see in the UI.

@andypols That makes sense. I believe that in #1043 we entirely remove the user of the repoName across the proxy, API and UI, other than for display purposes (as you do here in just the proxy using the same approach and some of the same code - but with more work on the tests).

TBH I thought about remove the project/user fields entirely as we retrieve the same info from the Github and Gitlab APIs... I was stalled in the end by the idea that you would want a manually entered one for standalone git repos or other SCM hosts we don't have a specific integration for + the fact someone could be relying on them in a plugin implementation. The project (organisation) field is easier to remove as thats a github/gitlab concept (and differs between them as Gitlab supports many levels of sub-orgs), which could come from the respective APIs (the field is returned but we don't currently need it, instead we just extract the URL to it in utils.tsx / fetchRemoteRepositoryData)

@kriswest I agree about removing the project/name fields when adding a repository. All you need is the clone URL

@andypols is any part of this PR NOT included in #1043 that I should be considering adding to it?

@kriswest no - I did to address this one issue and unblock our security team

@finos/git-proxy-maintainers @andypols and I have been chatting off to the side and essentially we have the same goal, we need these fixes to go in ASAP. He's raised this PR and #1110 in the hopes of speeding up resolution / adoption of some of the changes (+some work to improve the approach to building tests, particularly of the DB clients).

If #973 and #1043 go in soon, then we only need the test additions from these PRs and and I'll work (with @andypols) on getting them in. If there is likely to be an delay then #1109 and #1110 should be prioritised (and i'll have to maintain the others further). My preference is obviously for the former option to happen ASAP - but I don't want to unduly block another adopter if we can't get them merged ASAP.

@jescalada I'm conscious that I haven't looked at your PRs yet, but am happy to help in anyway that gets us to the end goal here ;-)

@kriswest We're waiting for a FINOS admin to help us unblock the PRs we talked about in the call. Maintainers cannot actually merge them directly through GitHub so we would need extended permissions or coordinate with an admin to get them in...

If it's not too much of a hassle to keep maintaining your PRs, I can take a look at #1109, #1110 and the other fix PRs by @andypols and make a patch release so their org can keep pentesting smoothly (which I believe is valuable to all of us).

As for my PRs, a quick look would be appreciated but they are pretty much ready to merge barring the permissions issue 🙂

I agree about removing the project/name fields when adding a repository. All you need is the clone URL

Seconding this for adding repos - it's simple and reliable. We can prepopulate the other values (repo name, organization, etc.) after fetching from the repo URL.

@jescalada – thanks for the test suggestions; I've added them. The one testing unknown URLs in mongo didn’t work at this level, though, since the unit test only checks queries against a mocked database. I’ve removed that one.

@andypols this can be rebased now - although the changes to how DBs are looked up in the DB by URL may have eliminated the need.

@kriswest I think there is a bug in your implementation of getRepoByUrl

export const getRepoByUrl = async (repoUrl: string): Promise<Repo | null> => {
  const collection = await connect(collectionName);
  const doc = collection.findOne({ name: { $eq: repoUrl.toLowerCase() } });
  return doc ? toClass(doc, Repo.prototype) : null;
};

Shouldn't this match on by url and not name?

@kriswest does it run for you on mongo? - the checked in code on main is not working for me.

The collection functions are promise based and are all returning empty documents.

For example:

const doc = collection.findOne({ name: { $eq: name } });

should be:

const doc = await collection.findOne({ name: { $eq: name } });

@kriswest I think there is a bug in your implementation of getRepoByUrl

export const getRepoByUrl = async (repoUrl: string): Promise<Repo | null> => {
  const collection = await connect(collectionName);
  const doc = collection.findOne({ name: { $eq: repoUrl.toLowerCase() } });
  return doc ? toClass(doc, Repo.prototype) : null;
};

Shouldn't this match on by url and not name?

I'll look into this - there were so many rounds of dealing with conflicts on this PR that there may be a merge issue there.

@andypols nope looks like a straight-up mistake, I'm not setup to test against mongo on my machine and, although it looks like there was a start on creating a mongo testing setup in the repo it was incomplete the last time I checked. Thats clearly something we need to change @finos/git-proxy-maintainers.

I've raised a bug fix PR (#1167), but can't test it until I have time to get some setup done here. If you have a moment to check we could move that forward today.

@finos/git-proxy-maintainers : I have updated this, please can you review and merge if happy. All that is left is a slight cleanup of the checkRepoInAuthorisedList code and additional tests

Thanks

The PR title/description could/should be updated before merge as the changes are now just the test additions (will make a better changelog entry).

@kriswest and @jescalada I thoughts these may be useful. Feel free to close the PR as the important stuff was added in #1043. I don't have the bandwidth to rename the PR and make the changes at the moment.